[ENH]: add list_fwhm option to create_susan_smooth #2233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2233 +/- ##
=========================================
- Coverage 72.31% 72.3% -0.02%
=========================================
Files 1182 1182
Lines 58947 58954 +7
Branches 8490 8494 +4
=========================================
- Hits 42628 42627 -1
- Misses 14935 14944 +9
+ Partials 1384 1383 -1
Continue to review full report at Codecov.
|
| mask_files = [mask_files] | ||
| multi_in_files = [in_file for in_file in in_files for fwhm in fwhms] | ||
| multi_mask_files = [mask_file for mask_file in mask_files for fwhm in fwhms] | ||
| multi_fwhms = [fwhm for fwhm in fwhms for in_file in in_files] |
There was a problem hiding this comment.
You always want to use the same order, or else you're not getting the Cartesian product. So this should be:
multi_fwhms = [fwhm for in_file in in files for fwhm in fwhms]| else: | ||
| smooth = pe.MapNode(interface=fsl.SUSAN(), | ||
| iterfield=['in_file', 'brightness_threshold', 'usans'], | ||
| name='smooth') |
There was a problem hiding this comment.
Drop the if statement, and just use the first part:
multi_inputs = pe.Node(
util.Function(function=cartesian_product,
output_names=['in_file', 'fwhm', 'usans', 'btthresh']),
name='multi_inputs') # It's okay if we never hook this up
smooth = pe.MapNode(
fsl.SUSAN(), iterfield=['in_file', 'brightness_threshold', 'usans'],
name='smooth')I'm also dropping the input_names from Function, as it is now redundant and a potential source of error. And I make the output match the expected input names to SUSAN. Finally, I give more descriptive names to your merge_out and median_out. You don't need to make all of these changes if you feel strongly about them, but at least consider them.
There was a problem hiding this comment.
ah, so if I exclude input_names, the Function interface infers the names of the inputs from the definition of the function? I agree with giving more descriptive names for merge_out and median_out. I also changed change the output_names to cart_<name>, to make it clear they are Cartesian lists (and not redefine usans and btthresh)
There was a problem hiding this comment.
Yup. Variable names are accessible parts of function objects. (Not true of output names. That's always just a tuple.)
Those changes sound good.
| @@ -767,11 +767,12 @@ def create_susan_smooth(name="susan_smooth", separate_masks=True): | |||
|
|
|||
| name : name of workflow (default: susan_smooth) | |||
| separate_masks : separate masks for each run | |||
| list_fwhms : multiple full wide half maximum smoothing kernels | |||
There was a problem hiding this comment.
I would drop this entirely. You can just now accept multiple floats, and treat a single float as a list of length 1.
| # replaces the functionality of a "for loop" | ||
| def cartesian_product(fwhms, in_files, mask_files, merge_out, median_out): | ||
| if type(in_files) == str: | ||
| in_files = [in_files] |
There was a problem hiding this comment.
There's already code to do this work for you.
from nipype.utils.filemanip import filename_to_list
in_files = filename_to_list(in_files)You can't use this for numbers, but to treat one FWHM as a list of length one:
fwhms = [fwhms] if isinstance(fwhms, (int, float)) else fwhms| if type(in_files) == str: | ||
| in_files = [in_files] | ||
| if type(mask_files) == str: | ||
| mask_files = [mask_files] |
There was a problem hiding this comment.
It looks like you don't actually use multi_mask_files, so I'd drop mask_files and multi_mask_files from this function entirely.
| susan_smooth.connect(inputnode, 'in_files', smooth, 'in_file') | ||
| susan_smooth.connect(inputnode, 'fwhm', smooth, 'fwhm') | ||
| susan_smooth.connect(median, ('out_stat', getbtthresh), smooth, 'brightness_threshold') | ||
| susan_smooth.connect(merge, ('out', getusans), smooth, 'usans') |
There was a problem hiding this comment.
I think this will be more readable with a multi-connect statement:
susan_smooth.connect([
(inputnode, multi_inputs, [('in_files', 'in_files'),
('fwhm', 'fwhms')]),
(median, multi_inputs, [(('out_stat', getbtthresh), 'btthresh')]),
(merge, multi_inputs, [(('out', getusans), 'usans')]),
(multi_inputs, smooth, [('in_file', 'in_file'),
('fwhm', 'fwhm'),
('btthresh', 'brightness_threshold'),
('usans', 'usans')]),
])There was a problem hiding this comment.
I think so too, but I didn't want to change the way they had it in case that was their coding standard, but with your recommendation I'll change it.
There was a problem hiding this comment.
My feeling is this workflow was written to be readable, as an example for writing them. I think having a bit of a mix where we choose the most readable at any given point is within the spirit of the thing.
|
|
||
| Inputs:: | ||
|
|
||
| inputnode.in_files : functional runs (filename or list of filenames) | ||
| inputnode.fwhm : fwhm for smoothing with SUSAN | ||
| inputnode.fwhm : fwhm for smoothing with SUSAN (float or list of floats) |
There was a problem hiding this comment.
This doc update is good, though. Let's keep that.
|
@effigies I have tested the workflow locally using a single off topic: Do you have any resources for learning how to write tests (unit tests, regression tests) for python repositories like nipype and fmriprep? |
|
The doctest doesn't actually get run during testing (see How about creating a niworkflows branch that pins against this PR, and then your FMRIPREP branch to that? In case you haven't updated the nipype pin before: git fetch upstream
git checkout -b pin/nipype_pr2233 upstream/master
sed -i -e 's/nipy\//jdkent\//' .gitmodules
git submodule sync
git submodule foreach git fetch origin
git submodule foreach git checkout origin/enh-create_susan_smooth
git add .gitmodules nipype
git commit -m 'PIN: jdkent/nipype feature branch'
git push -u origin pin/nipype_pr2233I think you're familiar with pinning fmriprep against a niworkflows branch? |
|
In fmriprep, our version of regression tests are really just running the entire workflow on truncated datasets with every commit. Our unit tests are sorely lacking. In nipype, if I'm adding a new feature, I try to look at the tests for nearby features (mostly using grep or searching in GitHub), and see what I can do to exercise the new behavior. Then I mostly depend on Satra to decide whether it's reasonable. And if you're fixing a bug, trying to create a test that fails before but passes after is a good strategy. But I don't have any references that I regularly recommend. Perhaps I should try to evaluate them some time. Might be worth asking on the brainhack Slack to see if people have suggestions. |
|
Oh, sorry. Before any of that git submodule init
git submodule update |
|
oops, probably should have done that first, I eventually modified your instructions from the ICA-AROMA commit. I believe it followed something like the following. So the important bit was |
|
@effigies - please merge this when ready. i got a little lost with the git submodule comments. |
|
Sounds good. (Submodule stuff is for downstream testing in an fmriprep branch.) |
|
is this all set? |
|
I haven't found the time to test it rigorously downstream (in FMRIPREP), but this does work as expected in simpler examples. |
|
I'm okay with merging.
…On Nov 7, 2017 18:45, "James Kent" ***@***.***> wrote:
I haven't found the time to test it rigorously downstream (in FMRIPREP),
but this does work as expected in simpler examples.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2233 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFF8hys5B3alnA4lOQcozG4KibWL2jqks5s0RWqgaJpZM4P7oer>
.
|
helps with nipreps/fmriprep#706
Changes proposed in this pull request
list_fwhmsoption tocreate_susan_smoothto allow a list of fwhms to be run, instead of one.